Skip to content

fix: use Grafast plan() instead of resolve() for downloadUrl field#1019

Merged
pyramation merged 1 commit intomainfrom
devin/1776716000-fix-download-url-grafast-plan
Apr 20, 2026
Merged

fix: use Grafast plan() instead of resolve() for downloadUrl field#1019
pyramation merged 1 commit intomainfrom
devin/1776716000-fix-download-url-grafast-plan

Conversation

@pyramation
Copy link
Copy Markdown
Contributor

Summary

The downloadUrl computed field on @storageFiles-tagged file types was always returning null because it used a traditional GraphQL resolve() function. In PostGraphile V5, Grafast's planning system does not invoke traditional resolve() on PG table type fields — it only plans them as column lookups. Since downloadUrl is a computed field (not a real DB column), the resolver was silently never called.

This replaces resolve() with a Grafast plan() function using lambda(), object(), and grafastContext() — the same pattern already used by requestUploadUrl and confirmUpload mutations in this plugin. The underlying business logic (status check → per-database config resolution → public CDN URL or presigned GET URL) is unchanged.

Plugin version bumped from 0.1.00.2.0.

Review & Testing Checklist for Human

  • Verify the Grafast plan pattern matches the mutation pattern in plugin.ts (~lines 217-403). The $parent.get('column')object()lambda() chain should mirror how requestUploadUrl accesses its input. This is the core correctness question.
  • Verify context key names: the old code accessed context.withPgClient and context.pgSettings directly. The new code uses grafastContext().get('withPgClient') and grafastContext().get('pgSettings'). Confirm these keys exist in the Grafast context object (they should — the mutations use the same pattern).
  • Confirm the conditional change is equivalent: old code used context.pgSettings ? context.withPgClient : null, new code uses if (withPgClient && pgSettings). Both guard against missing context, but the shape is slightly different.
  • Test end-to-end with the companion test PR (constructive-db#934). That PR has 3 download tests that exercise this field via GraphQL: private file presigned URL, entity-member download, and public CDN URL. All 32 tests pass locally with MinIO. This repo's CI does not directly exercise the download path.

Notes

  • There are no tests for the download URL field in this repo — coverage comes from the integration tests in constructive-db.
  • All types in the plan function are any, consistent with the existing mutation patterns in this plugin. Proper typing would require importing Grafast step types.
  • The silent catch {} fallback to global S3 config is preserved from the original code.

Link to Devin session: https://app.devin.ai/sessions/18879be982854a40abe5c9b915aa4a84
Requested by: @pyramation

PostGraphile V5's Grafast execution engine does not call traditional
resolve() functions on PG table type fields. The downloadUrl computed
field was returning null because its resolve() was never invoked.

Replace with Grafast plan() using lambda(), object(), and
grafastContext() — the same pattern used by requestUploadUrl and
confirmUpload mutations in the same plugin.

This fixes presigned GET URL generation for private files and
public CDN URL generation for public files.
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@pyramation pyramation merged commit 5c5c0b5 into main Apr 20, 2026
51 checks passed
@pyramation pyramation deleted the devin/1776716000-fix-download-url-grafast-plan branch April 20, 2026 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant